fix/sync_dev_after_v170 #348
Conversation
…e_market_issue fix/solve_stream_derivative_market_issue
…ator after chain upgrade v1.13
…base_values feat/update_gas_estimator_base_values
…e. Updated denoms INI files
Release/v1.7.0
…o fix/sync_dev_after_v170
WalkthroughThe pull request introduces several updates across multiple files, including the addition of OFAC restricted address validations, modifications to gas limit estimators, and adjustments in the Google API client library. Key changes include the refactoring of gas limit constants in the testing suite, ensuring consistency with the new estimators. The project version has been updated, and dependencies have been specified more strictly. Overall, these changes enhance the application's compliance, functionality, and maintainability. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AsyncClient
participant GasLimitEstimator
participant API
User->>AsyncClient: Request market updates
AsyncClient->>GasLimitEstimator: Calculate gas limits
GasLimitEstimator-->>AsyncClient: Return gas limits
AsyncClient->>API: Stream market data
API-->>AsyncClient: Send market updates
AsyncClient-->>User: Deliver market updates
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
pyproject.toml (1)
34-34: Consider the implications of pinning theprotobufdependency.Pinning the
protobufdependency to an exact version ("=5.26.1") can help maintain compatibility and stability in the SDK's functionality. However, it's important to note the potential drawbacks:
- Pinning to an exact version may prevent the project from automatically receiving bug fixes and improvements in future patch releases of
protobuf.- It's crucial to ensure that the pinned version has been thoroughly tested with the SDK to avoid any unexpected issues.
Recommendation:
- Periodically review and update the pinned version to benefit from bug fixes and improvements in the
protobufpackage.pyinjective/async_client.py (1)
Inconsistent implementation of
stream_marketacross the codebaseThe change from
stream_marketstostream_markethas been correctly implemented for the derivative stream inpyinjective/async_client.py. However, there are several issues that need to be addressed:
The spot stream API still uses
stream_markets(plural):
pyinjective/async_client.py:1816: await self.exchange_spot_stream_api.stream_markets(Several occurrences of
stream_marketsremain in test files and other parts of the codebase:
tests/test_async_client_deprecation_warnings.pypyinjective/client/indexer/grpc_stream/indexer_grpc_spot_stream.pytests/client/indexer/configurable_spot_query_servicer.pytests/client/indexer/stream_grpc/test_indexer_grpc_spot_stream.pyTo ensure consistency and prevent potential bugs:
- Review and update the spot stream API to use
stream_market(singular) for consistency with the derivative stream.- Update all related code, including test files, to reflect this change.
- Ensure that any client code using these APIs is also updated to maintain compatibility.
Analysis chain
Line range hint
2339-2346: Approve the change fromstream_marketstostream_market.The change aligns with the singular naming convention and may provide more granular control over which market to listen to for updates.
Verify that any existing code relying on the old
stream_marketsmethod has been updated to usestream_marketand handle single market streaming.Run the following script to search for usage of the old method:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `stream_markets` have been updated to `stream_market`. # Test: Search for the old method usage. Expect: No occurrences of `stream_markets`. rg --type python -A 5 $'stream_markets'Length of output: 69
Script:
#!/bin/bash # Description: Verify all usages of `stream_markets` have been updated to `stream_market`. # Search for the old method usage echo "Searching for 'stream_markets':" rg -n --no-heading 'stream_markets' # Search for the new method usage echo "Searching for 'stream_market':" rg -n --no-heading 'stream_market' # Fallback to grep if rg fails if [ $? -ne 0 ]; then echo "Fallback to grep:" echo "Searching for 'stream_markets':" grep -rn 'stream_markets' . echo "Searching for 'stream_market':" grep -rn 'stream_market' . fiLength of output: 2967
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
Files selected for processing (7)
- CHANGELOG.md (1 hunks)
- pyinjective/async_client.py (1 hunks)
- pyinjective/core/gas_limit_estimator.py (3 hunks)
- pyinjective/proto/google/api/client_pb2.py (3 hunks)
- pyproject.toml (2 hunks)
- tests/core/test_gas_limit_estimator.py (15 hunks)
- tests/core/test_message_based_transaction_fee_calculator.py (8 hunks)
Additional context used
LanguageTool
CHANGELOG.md
[duplication] ~6-~6: Possible typo: you repeated a word
Context: ...this file. ## [1.7.0] - 2024-09-18 ### Added - Added OFAC restricted addresses validations ...(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~14-~14: Possible typo: you repeated a word
Context: ...ter chain upgrade v1.13 ## [1.6.2] ### Fixed - Fixed issue in the `listen_derivative_market_...(ENGLISH_WORD_REPEAT_RULE)
Additional comments not posted (38)
pyproject.toml (1)
3-3: LGTM!The version bump to "1.8.0-pre" aligns with the PR objective of syncing the
devbranch after the v1.7.0 release. The minor version increment and the "pre" suffix are appropriate for a pre-release version in thedevbranch.tests/core/test_message_based_transaction_fee_calculator.py (7)
40-43: LGTM!The changes improve maintainability and accuracy by using constants from relevant classes instead of hardcoded values.
68-68: LGTM!Using
MessageBasedTransactionFeeCalculator.TRANSACTION_GAS_LIMITinstead of a hardcoded value improves maintainability.
90-90: LGTM!Using
MessageBasedTransactionFeeCalculator.TRANSACTION_GAS_LIMITinstead of a hardcoded value improves maintainability.
112-112: LGTM!Using
MessageBasedTransactionFeeCalculator.TRANSACTION_GAS_LIMITinstead of a hardcoded value improves maintainability.
142-142: LGTM!Using
MessageBasedTransactionFeeCalculator.TRANSACTION_GAS_LIMITinstead of a hardcoded value improves maintainability.
173-175: LGTM!The changes improve maintainability and accuracy by using constants from relevant classes instead of hardcoded values.
211-214: LGTM!The changes improve maintainability and accuracy by using constants from relevant classes instead of hardcoded values.
pyinjective/proto/google/api/client_pb2.py (4)
20-20: **** This is a generated code comment by the protocol buffer compiler. No action needed.
36-39: LGTM! The changes to the serialized indices of_CLIENTLIBRARYORGANIZATIONand_CLIENTLIBRARYDESTINATIONenums appear to be consistent and do not modify the actual enum values.
54-57: LGTM! The changes to the serialized indices of_PYTHONSETTINGSand the addition of the_PYTHONSETTINGS_EXPERIMENTALFEATURESmessage appear to be consistent. The newrest_async_io_enabledfield provides a feature flag for enabling asynchronous I/O for REST operations in Python.
58-73: LGTM! The changes to the serialized indices of various settings messages such as_NODESETTINGS,_DOTNETSETTINGS,_RUBYSETTINGS,_GOSETTINGS, and_METHODSETTINGSappear to be consistent and do not modify the actual message definitions.CHANGELOG.md (4)
5-7: LGTM!The addition of version 1.7.0 to the changelog with the new feature of validating OFAC restricted addresses is consistent with the changelog format and aligns with the PR objective of synchronizing the
devbranch after the v1.7.0 release. This feature enhances compliance and security measures, making it a notable addition to the project.Tools
LanguageTool
[duplication] ~6-~6: Possible typo: you repeated a word
Context: ...this file. ## [1.7.0] - 2024-09-18 ### Added - Added OFAC restricted addresses validations ...(ENGLISH_WORD_REPEAT_RULE)
9-11: LGTM!The addition of version 1.6.3 to the changelog with the fix for updating the reference gas cost in the gas limit estimator after the chain upgrade to v1.13 is consistent with the changelog format. This fix is crucial to ensure accurate gas estimations following the chain upgrade.
13-15: LGTM!The addition of version 1.6.2 to the changelog with the fix for the issue in the
listen_derivative_market_updatesmethod of theAsyncClientclass is consistent with the changelog format. This fix improves the reliability of market updates, making it a valuable addition to the project.Tools
LanguageTool
[duplication] ~14-~14: Possible typo: you repeated a word
Context: ...ter chain upgrade v1.13 ## [1.6.2] ### Fixed - Fixed issue in the `listen_derivative_market_...(ENGLISH_WORD_REPEAT_RULE)
Line range hint
1-16: Overall, the changes in the changelog look good!The changelog follows a consistent format, documenting notable changes, additions, and fixes across different versions. The changes align with the PR objective of synchronizing the
devbranch with themasterbranch after the v1.7.0 release.The static analysis hints about repeated words are false positives in the context of a changelog and can be safely ignored.
Tools
LanguageTool
[duplication] ~6-~6: Possible typo: you repeated a word
Context: ...this file. ## [1.7.0] - 2024-09-18 ### Added - Added OFAC restricted addresses validations ...(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~14-~14: Possible typo: you repeated a word
Context: ...ter chain upgrade v1.13 ## [1.6.2] ### Fixed - Fixed issue in the `listen_derivative_market_...(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~18-~18: Possible typo: you repeated a word
Context: ...ent` class ## [1.6.1] - 2024-08-07 ### Added - Added support for the following messages in t...(ENGLISH_WORD_REPEAT_RULE)
pyinjective/core/gas_limit_estimator.py (8)
15-16: Approve the increased gas limits for spot and derivative order creation.The gas limits for spot and derivative order creation have been increased to accommodate the higher computational resources required for processing these transactions. This change may impact transaction processing efficiency and cost.
18-18: Approve the increased gas limit for derivative order cancellation.The gas limit for derivative order cancellation has been increased to accommodate the higher computational resources required for processing these transactions. This change may impact transaction processing efficiency and cost for derivative order cancellations.
20-21: Approve the adjusted multipliers for POST ONLY orders.The multipliers for POST ONLY orders have been adjusted, with the spot order multiplier increasing from 0.5 to 0.62 and the derivative order multiplier decreasing from 0.5 to 0.35. These changes suggest a reevaluation of the gas consumption associated with these order types, possibly reflecting updated performance metrics or operational requirements. The adjustments may impact the gas consumption and cost for POST ONLY orders.
25-25: Approve the increased gas limit for general messages.The
GENERAL_MESSAGE_GAS_LIMITconstant in theGasLimitEstimatorclass has been increased from 15,000 to 25,000, indicating a significant adjustment in the baseline gas limit for general messages. This change may accommodate higher gas usage for general message handling and impact transaction processing efficiency and cost for general messages.
186-186: Approve the increased gas limit for batch update orders messages.The
MESSAGE_GAS_LIMITconstant in theBatchUpdateOrdersGasLimitEstimatorclass has been increased from 15,000 to 30,000, indicating a significant adjustment in the gas limit for batch update orders messages. This change may accommodate higher gas usage for batch update orders message handling and impact transaction processing efficiency and cost for batch update orders messages.
249-249: Approve the increased default gas limit for theExecGasLimitEstimatorclass.The
DEFAULT_GAS_LIMITconstant in theExecGasLimitEstimatorclass has been increased from 8,000 to 20,000, indicating a significant adjustment in the default gas limit for the execution of messages in this class. This change may accommodate higher gas usage for the execution of messages and impact transaction processing efficiency and cost for the execution of messages in theExecGasLimitEstimatorclass.
15-21: Overall assessment of the changes to gas limit constants and multipliers for spot and derivative orders.The modifications to the gas limit constants and multipliers for spot and derivative orders collectively suggest a broader strategy to accommodate higher gas usage across various order types. The changes indicate a shift towards higher gas limits, which may impact transaction processing efficiency and cost for these order types. It is important to monitor the impact of these changes on the system's performance and adjust accordingly if needed.
Line range hint
25-249: Overall assessment of the changes to gas limit constants for various message types and theExecGasLimitEstimatorclass.The modifications to the gas limit constants for general messages, batch update orders messages, and the default gas limit for the
ExecGasLimitEstimatorclass indicate a significant adjustment in the gas limits for these message types and the execution of messages in theExecGasLimitEstimatorclass. The increased gas limits may accommodate higher gas usage for these message types and the execution of messages, which may impact transaction processing efficiency and cost. It is important to monitor the impact of these changes on the system's performance and adjust accordingly if needed.tests/core/test_gas_limit_estimator.py (14)
59-60: LGTM!The changes align with the refactoring objective to replace hardcoded values with constants and estimators. The updated code looks good.
87-88: LGTM!The changes align with the refactoring objective to replace hardcoded values with constants and estimators. The updated code looks good.
118-119: LGTM!The changes align with the refactoring objective to replace hardcoded values with constants and estimators. The updated code looks good.
146-147: LGTM!The changes align with the refactoring objective to replace hardcoded values with constants and estimators. The updated code looks good.
181-182: LGTM!The changes align with the refactoring objective to replace hardcoded values with constants and estimators. The updated code looks good.
218-219: LGTM!The changes align with the refactoring objective to replace hardcoded values with constants and estimators. The updated code looks good.
275-276: LGTM!The changes align with the refactoring objective to replace hardcoded values with constants and estimators. The updated code looks good.
309-310: LGTM!The changes align with the refactoring objective to replace hardcoded values with constants and estimators. The updated code looks good.
343-344: LGTM!The changes align with the refactoring objective to replace hardcoded values with constants and estimators. The updated code looks good.
378-379: LGTM!The changes align with the refactoring objective to replace hardcoded values with constants and estimators. The updated code looks good.
398-399: LGTM!The changes align with the refactoring objective to replace hardcoded values with constants and estimators. The updated code looks good.
418-419: LGTM!The changes align with the refactoring objective to replace hardcoded values with constants and estimators. The updated code looks good.
438-439: LGTM!The changes align with the refactoring objective to replace hardcoded values with constants and estimators. The updated code looks good.
467-469: LGTM!The changes align with the refactoring objective to replace hardcoded values with constants and estimators. The updated code looks good.
devbranch with the latest changes from master after releasing version v1.7.0Summary by CodeRabbit
New Features
Bug Fixes
Performance Enhancements
Dependency Updates
Tests